Ensure properties are dereferenced before comparing equality#329
Ensure properties are dereferenced before comparing equality#329mikeharder merged 11 commits intoAzure:mainfrom
Conversation
|
I see the test fails with I took the spec: {
"swagger": "2.0",
"info": {
"title": "compatible-properties-ref",
"version": "1.0"
},
"paths": {
},
"definitions": {
"FooBarString": {
"type":"object",
"properties": {
"bar": {
"type":"string"
}
},
"allOf": [
{
"$ref": "#/definitions/FooBarStringRef"
}
]
},
"FooBarStringRef": {
"type":"object",
"properties": {
"bar": {
"$ref":"#/definitions/MyString"
}
}
},
"MyString": {
"type": "string"
}
}
}and pasted it here: I got no errors in both cases. Looking at that screenshot, both definitions have the same Interestingly, when I make Then |
|
OK I think I know what is the root-cause. The problem lies in openapi-diff logic. We throw error from here Which will happen if this line returns true:
On the failing test it evaluates to:
Now inside the
On the failing test it evaluates to:
which is As a result, the
which will evaluate like that:
Basically, the logic of |
Co-authored-by: Mike Harder <mharder@microsoft.com>
Fix the `incompatible properties` problem
|
@mikeharder I think you may want to change the PR title before merging to mention it fixes the bug. |



@johanste, @markcowl, @JeffreyRichter, @allenjzhang, @mikekistler: Please object if anything about this change looks wrong to you.
In summary, the change always calls
dereference()on a property with a$ref, before comparing equality.This looks pretty safe to me and @konrad-jamrozik, and passes the new unit test we wrote (plus all existing tests).